Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support Prettier v3 #901

Merged
merged 3 commits into from
Oct 14, 2023
Merged

Support Prettier v3 #901

merged 3 commits into from
Oct 14, 2023

Conversation

timdp
Copy link
Contributor

@timdp timdp commented Jul 7, 2023 •
edited
Loading

Prettier v3.0.0 was released this week. I tried to run prettier-eslint against it, and ran into:

TypeError: Expected `input` to be a `string`, got `object`
    at module.exports (node_modules/@prettier/eslint/node_modules/indent-string/index.js:11:9)
    at prettify (node_modules/@prettier/eslint/dist/index.js:133:37)
    at format (node_modules/@prettier/eslint/dist/index.js:113:20)
    at async node_modules/prettier-eslint-cli/dist/format-files.js:255:26

The indentation happens merely in a logging statement, but commenting it out just propagates the unexpected input to another function. The underlying issue is that prettier.format(), called from prettify(), is now async. Hence, the mysterious object is actually a Promise.

As a quick fix for that particular API change, it's sufficient to treat prettify as async and add an await. It's also called in two other places, but those are a return from an async function, so adding an await would be redundant. (I personally prefer to do it for clarity, but the linter rules won't let me.)

This is only a fix for this particular incompatibility. It does not:

  • upgrade prettier to v3.0.0: I tried, but then, the tests crash;
  • take into account other API changes: no idea if there are any.

However, it makes formatting work in my projects, and it's backwards compatible with v2, because you can safely await a string. Hence, it should at least be a step in the right direction.

(Incidentally, only calling indentString if the logger is actually enabled would probably provide a nice performance boost.)

@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 6780f52

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mattbrannon mattbrannon mentioned this pull request Jul 8, 2023
Closed
@Kreeg
Copy link

Kreeg commented Jul 20, 2023

When this will be merged?

@timdp
Copy link
Contributor Author

timdp commented Jul 25, 2023

For those wanting to experiment with this, I've published:

I was able to use the former as a drop-in replacement for the real prettier-eslint-cli while depending on [email protected], but YMMV.

@JounQin
Copy link
Member

JounQin commented Jul 25, 2023

upgrade prettier to v3.0.0: I tried, but then, the tests crash;

It seems this PR can not be merged as-is?

@timdp
Copy link
Contributor Author

timdp commented Jul 25, 2023

It seems this PR can not be merged as-is?

In the original PR, the logger was crashing. I didn't notice because I'd commented it out locally. I added an extra await and force-pushed 718e5f0. That one works for me; it's also the version I published under my npm scope.

@JounQin
Copy link
Member

JounQin commented Jul 25, 2023

@timdp

prettier-eslint/package.json

Line 32 in 1c0bdbf

"prettier": "^2.5.1",

But the prettier version in package.json does not change at all?

@timdp
Copy link
Contributor Author

timdp commented Jul 25, 2023

But the prettier version in package.json does not change at all?

My project depends on [email protected] directly, and this package contains complex logic that makes it prefer that over its own version.

As I mentioned in the issue description, upgrading the dependency makes the tests crash for an unknown reason:

node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

TypeError: A dynamic import callback was not specified.
    at new NodeError (node:internal/errors:405:5)
    at importModuleDynamicallyCallback (node:internal/modules/esm/utils:91:9)
    at Object.<anonymous> (/Users/timdp/Code/prettier-eslint/node_modules/prettier/index.cjs:600:23)
    at Runtime._execModule (/Users/timdp/Code/prettier-eslint/node_modules/jest-runtime/build/index.js:1205:24)
    at Runtime._loadModule (/Users/timdp/Code/prettier-eslint/node_modules/jest-runtime/build/index.js:805:12)
    at Runtime.requireModule (/Users/timdp/Code/prettier-eslint/node_modules/jest-runtime/build/index.js:662:10)
    at Runtime.requireActual (/Users/timdp/Code/prettier-eslint/node_modules/jest-runtime/build/index.js:683:17)
    at Object.<anonymous> (/Users/timdp/Code/prettier-eslint/src/__mocks__/prettier.js:1:23)
    at Runtime._execModule (/Users/timdp/Code/prettier-eslint/node_modules/jest-runtime/build/index.js:1205:24)
    at Runtime._loadModule (/Users/timdp/Code/prettier-eslint/node_modules/jest-runtime/build/index.js:805:12) {
  code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING'
}

Node.js v20.5.0

Jest gives me nightmares but I can do some more digging if the error doesn't ring a bell.

seanCodes added a commit to seanCodes/prettier-eslint that referenced this pull request Aug 4, 2023
@seanCodes
fix test setup; update dev dependencies; fix eslint warnings
a2decfc 
This fixes the testing issues for the PR to the upstream repo: prettier#901
@seanCodes seanCodes mentioned this pull request Aug 4, 2023
Merged
@seanCodes
Copy link
Contributor

@timdp @JounQin I've submitted a PR to Tim’s prettier-3 branch (timdp#1) that fixes the tests and includes the prettier 3 bump. Once it’s merged this PR should be good to go.

On a side note, it might be good to make prettier a peer dependency rather than a direct dependency. The prettier-eslint code actually supports prettier 2 and 3, so it could be nice for consumers to have the option to choose.

@seanCodes
Copy link
Contributor

seanCodes commented Aug 14, 2023 •
edited
Loading

@JounQin This change should now be ready for review 🙂

@Aitthi Aitthi mentioned this pull request Aug 28, 2023
Closed
@mbergkvist
Copy link

Any ideas when this will merged and released?

@seanCodes
Copy link
Contributor

@JounQin @danielwerg Bumping this. This issue makes the eslint-prettier plugin for VSCode useless with Prettier v3, so it's impacting productivity quite a bit. Is anything else blocking release?

@mrmarbury
Copy link

Please merge this

@monarchwadia
Copy link

monarchwadia commented Oct 5, 2023 •
edited
Loading

Just wanted to ask... When will this be merged?
Thank you for the great work you're doing for the community.

@JounQin JounQin merged commit 65c8b57 into prettier:master Oct 14, 2023
3 checks passed
@github-actions github-actions bot mentioned this pull request Oct 14, 2023
Merged
@JounQin
Copy link
Member

JounQin commented Oct 14, 2023 •
edited
Loading

Sorry for the delay, v16 has just been released! Before prettier-eslint-cli upgrade, you can install prettier-eslint@v16 manually with prettier-eslint-cli together, because prettier-eslint is an optional peer dependency for prettier-eslint-cli.

@JounQin
Copy link
Member

JounQin commented Oct 14, 2023

[email protected] has just been released!

@lthuong lthuong mentioned this pull request Nov 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielwerg danielwerg danielwerg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@timdp @Kreeg @JounQin @seanCodes @mbergkvist @mrmarbury @monarchwadia @danielwerg